JavaScript: Consolidate Express tests.#1018
Merged
Merged
Conversation
added 2 commits
March 1, 2019 09:39
Instead of having many small independent tests, we now just have a single test that pulls in all the individual tests and runs them together. Concretely, each `.ql` file has been turned into a `.qll` file with a query predicate corresponding to the original `select` clause and named after the original `.ql` file, plus a prefix `test_`. The newly added `tests.ql` imports all these `.qll`s. The individual `.expected` files have been concatenated together into `tests.expected`, each prefixed with the name of the corresponding query predicate. (This is the format that qltest produces for tests with multiple query predicates.)
ghost
reviewed
Mar 1, 2019
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Can we have a step that runs the autoformatter after the semantic transformation?
Minor, unrelated: import semmle.javascript.frameworks.ExpressModules is not required in the tests.
Author
Possibly, though for this PR I'll do it manually. I'll also fix the unnecessary import. |
Author
|
Tests have passed, so unless there are objections I'd like to remove the extra commits, autoformat and fix the spurious import pointed out by @esben-semmle. |
|
Go ahead. |
xiemaisi
force-pushed
the
js/consolidate-tests
branch
from
ceb9b07 to
8e34092
Compare
Author
|
Ping @esben-semmle. |
ghost
approved these changes
Mar 4, 2019
This was referenced Mar 5, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An alternative proposal to #999, achieving much the same effect but with a slightly less intrusive and (almost) entirely automated transformation.
I've written a little utility (based on the QL frontend) that, given a folder with
.qlfiles, does the following:selectclauses with semantically equivalent query predicates, where the name of the query predicate is the name of the.qlfile withtest_prepended to it..qlfiles to.qllfiles.tests.qlfile that imports all the.qllfiles created in the previous step..expectedfiles into one bigtests.expectedfile, with the result for each query predicate preceded by its name (which is the format used byqltestfor multi-query files).The tool does all of these steps in one go, resulting in the second commit of this PR. To make it easier to review, however, I've then reverted that commit and added individual commits corresponding to the steps above. I'll remove those extra commits once everyone is happy.
As mentioned above, the whole process is almost entirely automated, except that the tool currently sometimes fails to construct valid qualified names for QL types (a non-trivial problem in the presence of modules). Since this only affects three of our Express tests I've opted for fixing it manually for now, but I'll try to get that sorted as well and then make the tool available to others.